-
Notifications
You must be signed in to change notification settings - Fork 923
nginx 1.28.1 fixes #9711
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
julek-wolfssl
wants to merge
1
commit into
wolfSSL:master
Choose a base branch
from
julek-wolfssl:nginx-1.28.0
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
nginx 1.28.1 fixes #9711
+670
−284
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
5f54234 to
c37bd2f
Compare
Member
Author
|
Retest this please AgentOfflineException |
622feb1 to
1814bce
Compare
### `wolfssl/internal.h`
- **`InternalTicket` struct gains a flexible array member**: A new `peerCert[]` field (with a preceding `peerCertLen[2]`) is added to `InternalTicket`. This allows the peer's DER-encoded certificate to be stored directly inside the session ticket.
- **`ExternalTicket` struct becomes variable-length**: The `enc_ticket` field is changed from a fixed-size array to a flexible array member (`byte enc_ticket[]`). The `mac` field is removed from the struct — the MAC is now placed dynamically after the encrypted data in `enc_ticket`.
### `src/internal.c`
- The `GetRecordHeader` function now only adds `MAX_COMP_EXTRA` to the maximum allowed record size when `ssl->options.usingCompression` is true, tightening the length validation. The max fragment length extension check is now much stricter.
- **Peer certificate is serialized into the ticket**: During ticket creation, the code attempts to find the peer certificate from `ssl->peerCert` or from `ssl->session->chain` (fallback). If found and within `MAX_TICKET_PEER_CERT_SZ`, it's copied into `it->peerCert`. DTLS is explicitly excluded (peer cert length set to 0) to keep ticket size small for MTU constraints. If `HAVE_MAX_FRAGMENT` is defined and max fragment is not `MAX_RECORD_SIZE` for TLS 1.3, the cert is also skipped since `SendTls13NewSessionTicket` doesn't support fragmentation yet.
- **Peer certificate restoration from ticket**: On successful ticket decryption, if the ticket contains a peer certificate (`peerCertLen > 0`), it is decoded back into `ssl->peerCert` via `ParseCertRelative`/`CopyDecodedToX509`, and also added to `ssl->session->chain` via `AddSessionCertToChain`.
- The `CLEAR_ASN_NO_PEM_HEADER_ERROR` macro was rewritten to loop and remove all consecutive PEM no-start-line errors (not just the last one), wrapped in a `do { ... } while(0)` for safety.
- The `SendTicket` function is simplified to use `SendHandshakeMsg` to support fragmenting the larger ticket.
---
### `src/x509.c`
- `loadX509orX509REQFromPemBio` now accepts `TRUSTED_CERT_TYPE` in addition to `CERT_TYPE` and `CERTREQ_TYPE`.
- **Streaming BIO support**: When `wolfSSL_BIO_get_len()` returns ≤ 0 (e.g., pipes/FIFOs), the function no longer returns an error. Instead, it sets an initial buffer of `MAX_X509_SIZE` and dynamically grows (doubling) up to `MAX_BIO_READ_BUFFER` (`MAX_X509_SIZE * 16`) as data is read byte-by-byte.
- **Alternate footer detection**: For `TRUSTED_CERT_TYPE`, the PEM reader also checks for the regular `CERT_TYPE` footer (`-----END CERTIFICATE-----`) in addition to the trusted cert footer (`-----END TRUSTED CERTIFICATE-----`), so it can parse either format.
- Removed two lines that set `cert->srcIdx` to `SIGALGO_SEQ` offset. This makes `cert->srcIdx` reflect the end of parsed certificate data. This is used by `loadX509orX509REQFromBuffer` to detect where auxiliary trust data begins in trusted certificates.
---
### `src/ssl_sk.c`
- Added a `STACK_TYPE_X509_CRL` case to `wolfssl_sk_dup_data` that calls `wolfSSL_X509_CRL_dup` for deep-copying CRL stack elements. Previously, `STACK_TYPE_X509_CRL` fell through to the unsupported default case.
---
### `wolfssl/openssl/ssl.h`
- `sk_X509_dup` now maps to `wolfSSL_shallow_sk_dup` (was `wolfSSL_sk_dup`/deep copy). This matches OpenSSL's behavior where `sk_X509_dup` does a shallow copy.
- `sk_SSL_CIPHER_dup` similarly changed to `wolfSSL_shallow_sk_dup`.
---
### `src/ssl_api_cert.c`
- When `ssl->ourCert` is `NULL` and the SSL owns its cert, the function now checks if `ssl->ctx->ourCert` points to the same certificate (by comparing DER buffers). If so, it returns the ctx's `X509` pointer directly. This maintains pointer compatibility for applications (like nginx OCSP stapling) that use the `X509*` from `SSL_CTX_use_certificate` as a lookup key.
### `src/bio.c`
- When `wolfssl_file_len` returns `WOLFSSL_BAD_FILETYPE` (now returned for pipes/FIFOs), `wolfSSL_BIO_get_len` treats it as length 0 instead of propagating the error.
---
### `tests/test-maxfrag.conf` and `tests/test-maxfrag-dtls.conf`
- Removed `DHE-RSA-AES256-GCM-SHA384` test entries because the ClientKeyExchange doesn't fit in the selected max fragment length.
1814bce to
9192db1
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Depends on wolfSSL/wolfssl-nginx#31
wolfssl/internal.hInternalTicketstruct gains a flexible array member: A newpeerCert[]field (with a precedingpeerCertLen[2]) is added toInternalTicket. This allows the peer's DER-encoded certificate to be stored directly inside the session ticket.ExternalTicketstruct becomes variable-length: Theenc_ticketfield is changed from a fixed-size array to a flexible array member (byte enc_ticket[]). Themacfield is removed from the struct — the MAC is now placed dynamically after the encrypted data inenc_ticket.src/internal.cGetRecordHeaderfunction now only addsMAX_COMP_EXTRAto the maximum allowed record size whenssl->options.usingCompressionis true, tightening the length validation. The max fragment length extension check is now much stricter.ssl->peerCertor fromssl->session->chain(fallback). If found and withinMAX_TICKET_PEER_CERT_SZ, it's copied intoit->peerCert. DTLS is explicitly excluded (peer cert length set to 0) to keep ticket size small for MTU constraints. IfHAVE_MAX_FRAGMENTis defined and max fragment is notMAX_RECORD_SIZEfor TLS 1.3, the cert is also skipped sinceSendTls13NewSessionTicketdoesn't support fragmentation yet.peerCertLen > 0), it is decoded back intossl->peerCertviaParseCertRelative/CopyDecodedToX509, and also added tossl->session->chainviaAddSessionCertToChain.CLEAR_ASN_NO_PEM_HEADER_ERRORmacro was rewritten to loop and remove all consecutive PEM no-start-line errors (not just the last one), wrapped in ado { ... } while(0)for safety.SendTicketfunction is simplified to useSendHandshakeMsgto support fragmenting the larger ticket.src/x509.cloadX509orX509REQFromPemBionow acceptsTRUSTED_CERT_TYPEin addition toCERT_TYPEandCERTREQ_TYPE.wolfSSL_BIO_get_len()returns ≤ 0 (e.g., pipes/FIFOs), the function no longer returns an error. Instead, it sets an initial buffer ofMAX_X509_SIZEand dynamically grows (doubling) up toMAX_BIO_READ_BUFFER(MAX_X509_SIZE * 16) as data is read byte-by-byte.TRUSTED_CERT_TYPE, the PEM reader also checks for the regularCERT_TYPEfooter (-----END CERTIFICATE-----) in addition to the trusted cert footer (-----END TRUSTED CERTIFICATE-----), so it can parse either format.cert->srcIdxtoSIGALGO_SEQoffset. This makescert->srcIdxreflect the end of parsed certificate data. This is used byloadX509orX509REQFromBufferto detect where auxiliary trust data begins in trusted certificates.src/ssl_sk.cSTACK_TYPE_X509_CRLcase towolfssl_sk_dup_datathat callswolfSSL_X509_CRL_dupfor deep-copying CRL stack elements. Previously,STACK_TYPE_X509_CRLfell through to the unsupported default case.wolfssl/openssl/ssl.hsk_X509_dupnow maps towolfSSL_shallow_sk_dup(waswolfSSL_sk_dup/deep copy). This matches OpenSSL's behavior wheresk_X509_dupdoes a shallow copy.sk_SSL_CIPHER_dupsimilarly changed towolfSSL_shallow_sk_dup.src/ssl_api_cert.cssl->ourCertisNULLand the SSL owns its cert, the function now checks ifssl->ctx->ourCertpoints to the same certificate (by comparing DER buffers). If so, it returns the ctx'sX509pointer directly. This maintains pointer compatibility for applications (like nginx OCSP stapling) that use theX509*fromSSL_CTX_use_certificateas a lookup key.src/bio.cwolfssl_file_lenreturnsWOLFSSL_BAD_FILETYPE(now returned for pipes/FIFOs),wolfSSL_BIO_get_lentreats it as length 0 instead of propagating the error.tests/test-maxfrag.confandtests/test-maxfrag-dtls.confDHE-RSA-AES256-GCM-SHA384test entries because the ClientKeyExchange doesn't fit in the selected max fragment length.